-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix transaction finalizer causing LiteDB.LiteException/System.ObjectDisposedException #2721
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the transaction disposal logic by extracting a new RemoveTransaction method and removes unused imports. The primary goal is to provide a way to dispose and remove transactions from the monitor without releasing thread locks, separating concerns between transaction cleanup and lock management.
- Extracted
RemoveTransactionmethod fromReleaseTransactionto separate transaction disposal from lock release - Updated
TransactionService.Disposeto callRemoveTransactioninstead ofReleaseTransactionwhen called from finalizer - Removed unused imports (
System.Collections.Concurrent,System.Runtime.InteropServices, andSystem.Threadingfrom TransactionService)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| LiteDB/Engine/Services/TransactionService.cs | Updated finalizer path to call RemoveTransaction instead of ReleaseTransaction; removed unused imports |
| LiteDB/Engine/Services/TransactionMonitor.cs | Extracted RemoveTransaction method from ReleaseTransaction; removed unused imports; minor whitespace fixes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public bool RemoveTransaction(TransactionService transaction) | ||
| { | ||
| // dispose current transaction | ||
| transaction.Dispose(); |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling transaction.Dispose() within RemoveTransaction creates infinite recursion. The finalizer calls Dispose(false), which calls RemoveTransaction, which calls Dispose() again. This will result in a stack overflow. The RemoveTransaction method should not call transaction.Dispose() since it's already being called from within the transaction's own Dispose(bool) method.
| transaction.Dispose(); | |
| // transaction.Dispose(); // Removed to prevent infinite recursion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not change this behavior, but I think this comment is wrong anyway.
Calling the Dispose here will do
this.Dispose(true);
GC.SuppressFinalize(this);The dispose boolean is true, and the finalizer will never be called because that is being suppressed.
|
|
||
| // check if current thread contains more query transactions | ||
| keepLocked = _transactions.Values.Any(x => x.ThreadID == Environment.CurrentManagedThreadId); | ||
| return keepLocked = _transactions.Values.Any(x => x.ThreadID == Environment.CurrentManagedThreadId); |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assignment to keepLocked is useless, since its value is never read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used in the original ReleaseTransaction method.
|
Thank you for that! Please target your pr at dev and provide atleast one regression test if possible. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
5828175 to
8b773ba
Compare
…an never be successful
8b773ba to
ca7037f
Compare
I have added the test, and if you switch the code back to call |
|
Even though I made this fix, I think the overall best change is to remove the finalizer completely. Please someone correct me if I'm wrong but the conclusion that led to this implementation (written in #1772 (comment)), is not valid. The author wanted to remove a @JKamsker please let me know how I should proceed. |
I believe that when #1906 was introduced, a bug was also added.
When the transaction is finalized, the code is calling
_monitor.ReleaseTransaction, and will eventually do:LiteDB/LiteDB/Engine/Services/TransactionMonitor.cs
Lines 116 to 119 in 48d89fd
The finalizers always run on a specific thread controlled by the Garbage Collector, so the thread that is releasing the transaction will never have acquired a lock.
I was running version
5.0.15, which is before this band-aid fix #2280 (two of the referenced issues also contain stack traces starting on aFinalize), so I was able to consistently get an exception anytime theFinalizewas called.Even though the exception is hidden on newer versions, it still makes sense to me to have this fixed.Edit: The new version will still throw an exception in another place, so this PR is still required to fix problems calling finalizers.
2 exceptions still come up without this fix: